-
Notifications
You must be signed in to change notification settings - Fork 137
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use pkg_infos() to speed up rpm_build_info() #2287
Conversation
The existing approach of running shell commands might be easier for ongoing tinkering with which ones to use, and for spying on subprocesses (as I've been doing); and in this case, |
@chrstphrchvz Thanks for this rather nice simplification. I hope it does pan out that we can go this way, and it will be nice to start dropping the prior requirement of having to also be CentOS yum compatible as well as openSUSE dnf-yum compatible. As this is fairly critical path stuff, we really don't want to break updates to save a few seconds, or more like a single second on actual targeted hardware, could you take a look at how these changes pan out in our existing tests for this area of the code: https://github.com/rockstor/rockstor-core/blob/master/src/rockstor/system/tests/test_pkg_mgmt.py The following forum thread has info on how to run these once you have a working source build: https://forum.rockstor.com/t/built-on-opensuse-dev-notes-and-status/5662 We have to be super careful on the update code. Especially if it's just for performance reasons. As I indicated in response to another great suggestion of yours recent, I'm nervous about making changes in this area of the code if it's currently working. But if we have reasonable testing coverage and we can do an end to end test (any source install will consider any rpm install to be an update and will auto-wipe the db while at it) then it's far less of a worry and we get the goodness of speedups. Let us know how you get on with the unit test or if you can improve them if need be. As you see this area of the code is both old and new but definitely clunky and so all the more likely to hide surprises. But again this also makes it ripe for improvement such as your suggestion here. In summary I'm not happy saving a second or two for any risk to all users of the rpm update track. But I'm also not happy with the current state of some of that code. So we should have good coverage of these changes and tests against regressions as if we break updates we break the vast majority of our users ability to be able to update their way out of any other problem we also have to address bar being sub-optimal on the speed front. You may be interested to know that we are also planning to move to Python 3, which will in turn offer speed-ups: Thanks for submitting this pr and don't be put off by any means. I just wasn't sure if you were aware of our existing, all be it insufficient, tests; and the potential project failure a breakage in update can represent. We had one a few years ago and are still dealing with the fall out. See for context the nuance in: Cheers. |
Thanks for the response, will look into those. In this case, since I'm not sure whether dnf/yum output is intended more for human than machine readability and won't change over time (Rockstor already accommodates some differences), I would think it is more reassuring to rely on If there is concern that rpm might return different information about installed packages than dnf/yum, I am currently not aware of that being possible. |
@chrstphrchvz Re:
Yes, quite possibly. Let us know how you get on after looking at/running our current test as that may well help, both with proving your changes and identifying more what stuff is used for. Cheers. |
…rather than a string with "rockstor" hardcoded
In test_pkg_mgmt.py, the Or should this instead exercise the actual |
@chrstphrchvz Well done on making progress here.
@FroggyFlox rand into something similar in the network code. The neater solution would be to refactor, if possible, the tested code, such that each testable element only contains a single call to run_command. This may not be possible, but if it is then we further increase our potential to fully test without having overly complex (read fragile) tests in the first place. And hopefully further simplify the code under test also. Hope that helps. |
I think that can be done in this case by rewriting/extending |
@chrstphrchvz Apologies for the delayed response on this one.
That sounds just the ticket. Re 'types', at first glance, I'd say the input of multiple tags would fit well with being moved to a list. But whatever works really. But changing the nature of what is returned may end up opening a can of worms so bit by bit and see how you get on. Really looking forward to type hints in Python 3 but again, bit by bit. It would be nice to have better test coverage in this area because as stated previously, it's current state has been haunting us for a while and given it's critical nature we have to be very careful. Especially if we are just chasing performance. But if we can turn that into greater robustness/simplicity then great. Thanks again for looking so closely as this stuff. Quite the concern given it's pivotal nature in our ability to deliver fixes. |
I realized that if we want to run |
@chrstphrchvz I'll close this for now as we are now based on Py3.11: so you may want to pick-up on this effort from scratch again now. Thanks again for having a go at this. We have picked up quite the speed improvement of late so it may also be useful to use some of the new Python profiling that is now available to us, regarding finding our slow-spots as it were. I also suspect that our newer zypper can retrieve changelogs form uninstalled packages: if so that would be a great improvement as we can dump all our dnf nonsense just to get our changelog. Super confusing for folks when they see that installed but unable to do anything. Just a though. Thanks again for your improvements/speed-ups to date. Hope to see you again in these parts :). Cheers. |
Fixes: #2285